-
Notifications
You must be signed in to change notification settings - Fork 362
Better preserve timezones in ical #569
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…timezone info is present and update related specs
@sergioisidoro this is great work! I could really use this feature, do you need any help on getting this PR across the finish line? |
@wwahammy I'm fairly pessimistic about seeing this merged in this project. This is a breaking change, and the last commit to main has been done last year. There have been other people struggling with this issue with some atempts at solving it, and they have not been merged. At the moment I'm using my fork installing the gem through git, and I've been considering publishing my fork to rubygems for our own use, as we're starting to use recurrent events more and more, with iCal as serialization format with the API. But if you find any way of helping this through the finnish line, I would appreciate it. Or if we would find enough people to help with the maintenace of a separate fork? |
I don't think I have any more influence than you on the project. I'd be open to helping with the fork. |
Oh, one thing I had been meaning to ask: is the format you use for the timezones follow the iCal standard? I haven't looked into it so I wasn't sure but I wanted to know. On particular use-case is that, if we can serialize to standard iCal, we could use a postgres extensions supporting iCal to do actual queries based upon the values. |
Yes, and no. The standard does not specify a specific time zone format
https://www.ietf.org/rfc/rfc2445.txt However, it suggests the Olson tz database - ie the timezone database matinained by IANA. The current example of the timezone identifier can be seen here: https://en.wikipedia.org/wiki/List_of_tz_database_time_zones And as you can see there are same abbreviations for multiple timezones (the old implementation). From postgres docs I can see that the "full timezome" format is supported as input ![]() |
Hello, Thank you for your work on this! Am keen to keep this project alive. I will try to put some time aside this weekend to review this PR properly and see if there's a way we can get this merged. |
Thank you for all of the work you do @pacso! Totally get that you're doing this on the side and that's a lot of work. If you're open to it, it might make sense to work together on developing a path forward that spreads the workload more so you're not responsible for so much. We're all benefiting so we should all pitch-in after all. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I self reviewed the PR :) there were a few typos and a small nitpick. This work was done a bit in a hurry to fix our bugs with the lost time in serialization.
Let me know if there's anything else that needs attention on this PR, and I'll fixe everything in one go
time = time.dup.utc if force_utc | ||
|
||
# Keep timezone. strftime will serializer short versions of time zone (eg. EEST), | ||
# which are not reversivible, as there are many repeated abbreviated zones. This will result in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# which are not reversivible, as there are many repeated abbreviated zones. This will result in | |
# which are not reversible, as there are many repeated abbreviated zones. This will result in |
# Convert to UTC as TZID=+xxxx format is not recognized by JS libraries | ||
warn "IceCube: Time object does not have timezone info. Assuming UTC: #{caller(1..1).first}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Convert to UTC as TZID=+xxxx format is not recognized by JS libraries | |
warn "IceCube: Time object does not have timezone info. Assuming UTC: #{caller(1..1).first}" | |
# Convert to UTC as TZID=+xxxx format is not recognized by JS libraries | |
warn "IceCube: Time object does not have timezone info. Coercing into UTC: #{caller(1..1).first}" |
Schedule.from_hash data | ||
end | ||
|
||
def self.deserialize_time_with_tzid(time_value, tzid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should maybe be moved into the TimeUtil class since it's a wrapper for the TimeUtil.deserialize_time
rescue ArgumentError | ||
# If the timezone is invalid, fall back to standard deserialization | ||
# Perhaps we want to log this? | ||
TimeUtil.deserialize_time(time_value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we issue a warning here as well?
Trying to approach #568
May also fix:
#535